fix: recover tmux terminal streams after server restart - #597
fix: recover tmux terminal streams after server restart#597TiantianFlow wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Restores tmux terminal output and inbox delivery after server restarts.
Changes:
- Reattaches FIFO readers and tmux pipe targets.
- Seeds terminal status from pane snapshots.
- Reconciles pending inbox messages and adds recovery tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
terminal_service.py |
Implements persisted terminal recovery. |
status_monitor.py |
Adds snapshot-based status seeding. |
api/main.py |
Runs recovery and reconciliation during startup. |
test_terminal_recovery.py |
Tests recovery wiring and backend skipping. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| _rearm_pipe() | ||
| status_monitor.seed_from_snapshot(terminal_id, snapshot) |
| snapshot = backend.get_history( | ||
| session_name, window_name, tail_lines=PIPE_LIVENESS_TAIL_LINES | ||
| ) |
| # Recovery publishes initial status before the consumer tasks get their first | ||
| # scheduling turn. Reconcile once here so a callback already pending at | ||
| # restart is not left waiting for the periodic sweep. | ||
| await asyncio.to_thread(inbox_service.reconcile_orphaned_messages, registry) |
| list_terminals.return_value = [ | ||
| {"id": "abc12345", "tmux_window": "tech_lead-a1b2"} | ||
| ] | ||
| status_monitor.seed_from_snapshot.return_value = TerminalStatus.COMPLETED |
| self._buffers[terminal_id] = output[-state_buffer_max:] | ||
| # A persisted terminal is quiescent at recovery time. Do not let a | ||
| # prior process's debounce state suppress its first real status. | ||
| self._bursting[terminal_id] = False | ||
| detected = self._detect_status(terminal_id, output[-state_buffer_max:]) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #597 +/- ##
=======================================
Coverage ? 91.26%
=======================================
Files ? 182
Lines ? 24463
Branches ? 0
=======================================
Hits ? 22326
Misses ? 2137
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@TiantianFlow thanks for your contribution. Do you have a chance to address feedbacks, fix the CI issue and resolve the conflicts ? |
haofeif
left a comment
There was a problem hiding this comment.
Reviewed at d9597de.
The diagnosis is right and the layering is good. Re-arming with stop_pipe_pane then pipe_pane (rather than a bare pipe-pane -o toggle) matches the canonical wiring at terminal_service.py:517-534 and is exactly right for an inherited stale target; the supports_event_inbox() skip is correct (herdr's pipe_pane is a no-op); per-terminal exception isolation keeps one bad pane from taking down startup; and the s=/w=/p= default-argument binding in the closures avoids the late-binding trap. list_terminals_by_session does return tmux_window (database.py:1262), so the unguarded subscript is safe.
Three things block merge.
Mechanical
- CI is red.
black --checkwantstest/services/test_terminal_recovery.py:24-26collapsed onto one line — reproduced locally, one-line fix. - The branch is
CONFLICTINGagainstmainand 20 commits behind.
Behavioural — see the inline P1. seed_from_snapshot writes into the raw rolling buffer but never primes the pyte screen, so for the five providers that set supports_screen_detection = True (claude_code, codex, kimi_cli, opencode_cli, antigravity_cli) — with CAO_PYTE_STATUS defaulting to true (constants.py:228) — a worker that was mid-turn at restart never leaves PROCESSING. That is the same stranded-callback symptom this PR is fixing, just relocated. Your live validation exercised the already-idle Tech Lead, which is the one case that works.
Your own description says this needs a split-pane guard and failure-path coverage "before marking this ready" — that reads as not-ready-to-merge, and the PR isn't a draft.
I re-checked Copilot's points and disagree with one:
status_monitor.py:510("rendered snapshot into the raw detector misclassifies recovery") — I could not reproduce this. Feeding the rendered viewports fromtest_claude_code_unit.py::TestClaudeCodeScreenDetectionandtest_antigravity_cli_unit.pythroughget_status()returned the same status asget_status_from_screen()in every case I tried, including the busy-spinner viewport and the stale-footer regression frame. The raw detectors tolerate rendered input. The real defect is the unprimed screen, not the detector choice — please don't fix this by routing the seed throughget_status_from_screen; that leaves the screen just as blank.terminal_service.py:240(snapshot captured before the re-arm) — confirmed, and the P1 remedy subsumes it: capture after_rearm_pipe().api/main.py:1102— confirmed.list_pending_receiver_ids_older_than()sits outsidereconcile_orphaned_messages's per-terminaltry(inbox_service.py:168), so a query failure propagates out ofasyncio.to_threadand aborts the lifespan. The daemon atapi/main.py:210-213wraps it; this one-shot call should too. Low likelihood, cheap.
One more thing worth knowing: INBOX_RECONCILE_GRACE_SECONDS is 30, so the startup sweep only adopts messages already pending for 30s+. A callback queued seconds before the restart still waits for the periodic daemon — worth a line in the PR description so the guarantee isn't oversold.
Tests run at this head: test/services/{test_terminal_recovery,test_status_monitor,test_fifo_reader,test_inbox_service}.py → 89 passed.
| # A persisted terminal is quiescent at recovery time. Do not let a | ||
| # prior process's debounce state suppress its first real status. | ||
| self._bursting[terminal_id] = False | ||
| detected = self._detect_status(terminal_id, output[-state_buffer_max:]) |
There was a problem hiding this comment.
[P1] Seeding bypasses the pyte screen, so a worker that was mid-turn at restart stays PROCESSING forever
seed_from_snapshot writes the snapshot into self._buffers and calls _detect_status — the raw path. It never touches self._screens. But _process_chunk:139-142 routes every provider with supports_screen_detection = True through the pyte screen whenever CAO_PYTE_STATUS is on, and that is the default (constants.py:228). Five providers opt in, including claude_code.
So after recovery the pyte screen is blank. The next real chunk off the re-armed FIFO is composited onto nothing, and TUI output is overwhelmingly cursor-addressed in-place repainting — which is the entire reason the screen path exists.
Reproduced at d9597de (pane mid-turn at restart; Ink then repaints row 2 in place to finish the turn):
A) PR #597 status_monitor.seed_from_snapshot()
screen primed at recovery : False
viewport after repaint : ['✻ Crunched for 12s']
seeded -> final status : processing -> processing
InboxService delivers? : False
B) replay through _process_chunk (the existing primitive)
screen primed at recovery : True
viewport after repaint : ['● Working on the task', '✻ Crunched for 12s',
'──────────', '❯', '──────────']
seeded -> final status : processing -> completed
InboxService delivers? : True
In (A) the truncated viewport yields UNKNOWN, which _apply_detection correctly suppresses because a known status is already latched — so the terminal is pinned at PROCESSING. An idle agent emits nothing further, so nothing ever re-triggers detection. The pending callback is never delivered, and only another restart clears it. That is precisely the failure this PR set out to fix; it just moved from "was idle at restart" to "was busy at restart", which is the more likely restart scenario.
The codebase already solved this. fifo_reader._rearm_stalled_pipe:686-688 recovers a dead forwarder by replaying the pane through the normal pipeline, and its docstring even documents the \r\n requirement:
replay = content.replace("\n", "\r\n")
bus.publish(f"terminal.{terminal_id}.output", {"data": replay})That \r\n is not cosmetic — get_history joins with a bare \n and pyte defaults LNM off, so raw capture-pane output staircases:
bare \n: \r\n:
|● Working |● Working
| ✻ Cultivating… (12s) |✻ Cultivating… (12s)
| ──────── |────────
Please reuse that primitive instead of adding a parallel one. Keeping it synchronous (which you need, so the seed lands before the reconcile_orphaned_messages call at api/main.py:1102) is easy — _schedule_screen_detection detects on the rising edge inline when _bursting is False, which you already set:
with self._lock:
self._buffers[terminal_id] = ""
self._screens.pop(terminal_id, None)
self._bursting[terminal_id] = False
# One code path for both detectors: primes the pyte screen, fills the
# raw buffer, and picks the capability-correct detector.
self._process_chunk(terminal_id, output.replace("\n", "\r\n"))
return self.get_status(terminal_id)And take the snapshot after _rearm_pipe() in terminal_service.py (Copilot's point at :240) so output produced during reattachment is included rather than lost to the old FIFO.
Whichever shape you land on, this needs a test that actually runs the real StatusMonitor — see my note on the test file.
| get_backend().pipe_pane(s, w, p) | ||
|
|
||
| try: | ||
| fifo_manager.create_reader(terminal_id, pane_probe=_probe_pane, rearm=_rearm_pipe) |
There was a problem hiding this comment.
[P2] Recovered idle terminals trip the cold-start watchdog and get permanently dropped from it
create_reader unconditionally sets _ever_delivered[terminal_id] = False and _registered_at[terminal_id] = now (fifo_reader.py:180-182). For a newly launched terminal that is a correct assumption — a starting CLI always emits something. For a recovered idle terminal it is wrong: the pane is quiescent by definition, so the FIFO will never deliver a byte.
The cold-start branch at fifo_reader.py:490-497 then matches on exactly that shape — not ever_delivered and past the grace window and content.strip() (a recovered pane is full of prior conversation) — and concludes "the forwarder never started, full stop." Note the self-ROAST comment at :498-508: the replay publishes straight to the bus and never flips _ever_delivered, so each attempt re-qualifies.
Reproduced at d9597de with the exact probe/rearm pair this function registers and an idle pane:
grace=3.0s max_cold_start_attempts=5
after create_reader: _ever_delivered=False enrolled=True
tick 1: rearms=1 still_enrolled=True
...
tick 5: rearms=5 still_enrolled=True
tick 6: rearms=5 still_enrolled=False
ERROR pipe-pane forwarder for terminal t1 never started delivering after 5
cold-start re-arm attempts — giving up and dropping it from the
liveness watchdog
So every recovered idle supervisor — the exact population this PR targets — gets ~5 spurious stop_pipe_pane/pipe_pane cycles plus 5 full-pane replays in the first ~20s (PIPE_LIVENESS_CHECK_INTERVAL_S 4.0s), an ERROR log, and is then unenrolled from the issue #388 liveness watchdog for the life of the process. If its pipe genuinely stalls later, nothing repairs it — silently, since the give-up already logged and won't log again.
P2 rather than P1 because the immediate effect is loud and benign-to-helpful; what's lost is a safety net, quietly.
Recovery knows the pipe was just re-armed against a live pane, so the cold-start check is not applicable here. Give create_reader a way to say so and have recovery pass it — the ordinary divergence check still protects these terminals:
# fifo_reader.create_reader(..., assume_delivered: bool = False)
self._ever_delivered[terminal_id] = assume_delivered
# terminal_service, recovery path
fifo_manager.create_reader(
terminal_id, pane_probe=_probe_pane, rearm=_rearm_pipe, assume_delivered=True
)| backend.list_sessions.return_value = [{"id": "cao-live"}] | ||
| backend.get_history.return_value = "❯ ready" | ||
| backend_getter.return_value = backend | ||
| list_terminals.return_value = [ |
There was a problem hiding this comment.
[P2] black fails here (CI red), and mocking status_monitor leaves the new logic completely untested
The Code Quality job fails on this file. Reproduced locally with uv run black --check:
- list_terminals.return_value = [
- {"id": "abc12345", "tmux_window": "tech_lead-a1b2"}
- ]
+ list_terminals.return_value = [{"id": "abc12345", "tmux_window": "tech_lead-a1b2"}]Separately — and this is why the P1 shipped unnoticed — the test patches terminal_service.status_monitor wholesale, so status_monitor.seed_from_snapshot.assert_called_once_with(...) only asserts that a MagicMock was called. seed_from_snapshot is referenced nowhere else in test/, so the real implementation is executed by zero tests. What the suite currently pins is the call wiring, not the recovery.
At minimum, add a test that drives the genuine StatusMonitor with a real provider:
def test_seed_primes_screen_so_a_mid_turn_pane_still_reaches_completed():
sm, prov = StatusMonitor(), ClaudeCodeProvider("t1", "s", "w")
busy = "\n".join(["● Working", "✻ Cultivating… (12s)", "─" * 60, "❯ ", "─" * 60])
with patch("...status_monitor.provider_manager") as pm, \
patch("...status_monitor.bus", MagicMock()):
pm.get_provider.return_value = prov
assert sm.seed_from_snapshot("t1", busy) == TerminalStatus.PROCESSING
sm._process_chunk("t1", "\x1b[2;1H\x1b[2K✻ Crunched for 12s") # in-place repaint
sm._apply_detection("t1", sm._detect_screen("t1", prov))
assert sm._last_status["t1"] == TerminalStatus.COMPLETED # fails on d9597deThe failure paths in recover_persisted_terminal_output_streams are also untested — get_history raising (stale DB row), create_reader raising, and the list_sessions guard — and those except blocks are the load-bearing part of the "must never prevent server startup" contract in the docstring.
Problem
cao-servercan restart while tmux-backed terminals intentionally remain alive. The old process's FIFO reader threads disappear, but tmux continues forwarding pane output to the old FIFO target. The restarted server therefore receives no output for those persisted terminals, leaves their status asunknown, and cannot deliver pending worker callbacks because InboxService only delivers to ready terminals.This was reproduced with a live Claude Tech Lead: two worker reports remained
pendingeven though the Tech Lead was visibly idle.Fix
For tmux-backed terminals at API startup:
pipe-panetarget;Event-inbox (Herdr) backends are explicitly skipped because they do not use tmux/FIFO output capture.
Validation
Automated
2 passed.These are deliberately narrow unit tests; they do not yet exercise end-to-end inbox delivery or failure/multi-pane cases.
Live end-to-end validation
cao-server; the existing Claude Tech Lead and OpenCode Builder tmux panes remained running.unknowntocompleted.Scope / follow-up
This is a tmux restart-recovery fix. CAO's intended topology is one terminal per window/pane. Before marking this ready, follow up with a guard for manually split panes plus end-to-end/failure-path coverage.